Redact credential field names that logging missed - #1270
Merged
Paul Lizer (paullizer) merged 3 commits intoAug 18, 2026
Conversation
CodeQL reported five high-severity clear-text logging alerts against the
shared sinks in functions_appinsights.py. They were not false positives.
log_event sanitizes its inputs before they reach a sink, but the redaction
decision for structured properties came from _is_sensitive_log_key, which
matched a fixed list of substrings. Several credential field names this
codebase actually uses contained none of those substrings, so their values
were logged in clear text.
The significant one is auth_key, the field the action connection-test
routes use for the caller-supplied secret, along with the plugin manifest's
auth.key, which plugin.schema.json describes as holding connection strings
and service principal passwords. Reproduced before the fix:
log_event("probe", extra={"auth_key": "<secret>"})
-> [LOG] probe -- {'auth_key': '<secret>'}
Eighteen credential key names were affected, including pwd, key_pair,
master_key, primary_key, secondary_key, encryption_key, signing_key,
session_key and storage_key.
Widening the match to any key containing "key" was not acceptable because
it would redact benign configuration such as key_encoding,
key_prefix_hints and partition_key_path and strip diagnostic value from
logs. Instead this adds the missing credential fragments and a separate
exact-match list for names that carry a secret only when they are the whole
key, so the fix stays surgical.
Also in this change:
- Completed the v0.250.047 CosmosClient import-binding cleanup in the two
remaining helper scripts. No direct CosmosClient imports remain in the
repository. deployers/version.txt bumped because deployers/ is touched.
- Restored test_privacy_logging_telemetry_audit.py, which had been failing
since v0.242.072 because it asserted an exact config.py version and never
reached its assertions. It now asserts a version floor per the
repository's version-assertion guidance.
Validation: new regression test passes 6/6 and was confirmed to fail
without the fix, reporting all 18 unredacted names and a reproduced
auth_key leak. The revived privacy audit passes 5/5. Route policy 12/12,
RocksDB 10/10, Cosmos 4/4, Yamcs 14/14. Pre-existing failures in unrelated
logging suites were compared against Development and are unchanged.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Development shipped Test Connection for eight more action types and claimed VERSION 0.250.217, which collided with this fix. Conflict resolved: - release_notes.md: the v0.250.217 heading sat above the conflict block, so both sides inherited it. Split it so Development's action connection tests keep v0.250.217 and this logging fix moves to its own v0.250.218 section above them. Renumbered alongside it: config.py VERSION, the fix document, and the functional test version headers. Development's new work does not touch functions_appinsights.py, so the credential redaction change is intact. Validation after the merge: credential redaction 6/6, privacy logging audit 5/5, route policy 12/12, and Development's new action connection test suites 5/5, 4/4 and 5/5. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CodeQL flagged py/unused-import on the new test file. The types module was carried over from the stub pattern in the neighbouring privacy audit test but is not used here. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes all 7 CodeQL findings left open after #1266. CodeQL now passes with zero open alerts.
The high-severity logging alerts turned out to be real, not false positives — I verified the leak empirically before writing any fix.
The actual bug
log_eventsanitizes its inputs before they reach any sink, so this looked like a false positive at first. It isn't. The redaction decision for structured properties came from_is_sensitive_log_key, which matched a fixed list of substrings. Several credential field names this codebase actually uses contain none of those substrings._normalize_log_keystrips punctuation, soauth_key/authKey/auth-keyall normalize toauthkey— and the fragment list hadaccountkey,apikey,privatekey,subscriptionkey, but notauthkey.Reproduced on
Developmentbefore the fix:That matters because
auth_keyis the field the action connection-test routes use for the caller-supplied secret, and the plugin manifest'sauth.keyis described inplugin.schema.jsonas holding "the secret value for the plugin ... such as a SQL connection string, a password for a service principal."A value under one of these keys was only redacted by luck — when the value itself happened to match
SECRET_ASSIGNMENT_RE(e.g. a connection string containingPassword=). A bare API key or token was emitted verbatim.18 credential key names were affected, including
pwd,key_pair,master_key,primary_key,secondary_key,encryption_key,signing_key,session_key, andstorage_key.The fix, and why it's shaped this way
The obvious fix — treat any key containing
keyas sensitive — is wrong. It would redactkey_encoding,key_prefix_hints, andpartition_key_path, stripping diagnostic value from logs.So this adds two things:
authkey,masterkey,keypair,encryptionkey,signingkey,sessionkey,storagekey,primarykey,secondarykey)key,keys,pwd,pass,passphrase,sig,signature){"auth_key": "<secret>"}***REDACTED***{"auth": {"key": "<secret>"}}secret=***REDACTED***{"pwd": "<secret>"}***REDACTED***{"key_encoding": "utf8"}{"partition_key_path": "/id"}This is timely:
Developmentjust shipped Test Connection for eight more action types, all of which handle caller-supplied secrets.Also included
py/import-of-mutable-attribute×2 — completed the v0.250.047 import-binding cleanup in the two scripts it missed, using that fix's documented pattern. No directCosmosClientimports remain anywhere in the repo.deployers/version.txtbumped per repo policy sincedeployers/is touched.test_privacy_logging_telemetry_audit.pyhas been failing since v0.242.072 because it asserted an exactconfig.pyversion, so it errored before reaching a single assertion. That's the anti-pattern the repo's own version-assertion guidance forbids. Switched toassert_app_version_at_least. It's the privacy audit for the exact code this PR changes, so it was worth reviving rather than leaving silently red — it now passes 5/5.Validation
test_log_credential_key_redaction.py(new)test_privacy_logging_telemetry_audit.py(revived)route_tests/Development's new work)The new test was confirmed to fail without the fix, reporting all 18 unredacted key names and reproducing the
auth_keyleak — so it's a real regression guard, not a tautology.I also compared every logging-suite failure against unmodified
Developmentby exit code: all deltas identical, zero regressions. Several logging tests fail onDevelopmenttoday for unrelated environment reasons.Separately: one unrelated red test
test_logging_tag_standardization.pycurrently fails onDevelopment— tagsAUTH_CALLBACK,MIXED_SOURCE_ANALYZE,WORKFLOW_ALERTS,YAMCS_PLUGINare missing fromdocs/reference/logging-tags.md. It's a small doc addition, but unrelated to these alerts, so I kept it out of this PR to preserve scope. Happy to fix it in a follow-up.